-
Notifications
You must be signed in to change notification settings - Fork 91
Improve search #1925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Improve search #1925
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1925 +/- ##
==========================================
- Coverage 99.02% 95.28% -3.75%
==========================================
Files 18 17 -1
Lines 1435 1442 +7
Branches 303 278 -25
==========================================
- Hits 1421 1374 -47
- Misses 14 68 +54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughThis update refactors search-related APIs and types across the codebase. It unifies search method signatures to accept a single options object, introduces new and revised TypeScript types for search parameters and responses, and reworks the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MeiliSearch
participant Index
Client->>MeiliSearch: multiSearch(MultiSearch)
MeiliSearch->>MeiliSearch: Detect MultiSearch or FederatedSearch
alt MultiSearch
MeiliSearch->>MeiliSearch: Return Promise<SearchResults>
else FederatedSearch
MeiliSearch->>MeiliSearch: Return Promise<FederatedSearchResult>
end
Client->>Index: search({ q, ...options })
Index->>Index: POST /indexes/:uid/search with searchQuery object
Index-->>Client: Promise<SearchResult<T>>
Client->>Index: searchGet({ q, ...options })
Index->>Index: GET /indexes/:uid/search with searchQuery as query params
Index-->>Client: Promise<SearchResult<T>>
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧬 Code Graph Analysis (3)src/types/search-response.ts (3)
src/indexes.ts (6)
src/types/types.ts (2)
🔇 Additional comments (12)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (11)
src/meilisearch.ts (1)
220-238
: Consider harmonising parameter naming & add a short JSDoc blockThe overload signatures use the parameter identifiers
multiSearch
/federatedSearch
, whereas the implementation that follows accepts a genericbody
.
Although this is perfectly valid TypeScript, the sudden change of identifier breaks the mental mapping for a reader and hurts IDE “rename-symbol” refactors.Also, the public method currently lacks any JSDoc even though the rest of the class is meticulously documented.
- /** {@link https://www.meilisearch.com/docs/reference/api/multi_search} */ - multiSearch( + /** + * Performs a multi-index or federated search request. + * {@link https://www.meilisearch.com/docs/reference/api/multi_search} + */ + multiSearch([Naming consistency & documentation]
tests/search.test.ts (2)
70-74
: Potentially incomplete network clean-up inafterAll
beforeAll
enables the network feature with a non-emptyself
/remotes
object.
afterAll
resets the network with an empty object, but does not disable the feature itself (network: false
).If other suites run after this one, the residual toggle may leak state and provoke 401s or unexpected routing. Consider restoring the previous value, e.g.
await index.delete().waitTask(); -await client.updateNetwork({}); +await client.updateExperimentalFeatures({ network: false }); +await client.updateNetwork({});
436-450
: Guard against undefined_vectors
to avoid false positives
assertIsVectors
assumesvectors
is always defined, yet the API returnsnull
(or absence) whenretrieveVectors=false
. Calling this helper accidentally in such cases would throw in the helper instead of failing the test meaningfully.-function assertIsVectors(vectors: SearchHit<Film>["_vectors"]) { +function assertIsVectors(vectors: SearchHit<Film>["_vectors"] | undefined) { + assert.isDefined(vectors, "expected _vectors to be present in hit");tests/utils/test-data/films.ts (1)
1-14
: Reduce repetition & bundle size by extracting common literalsThe dataset repeats the exact same Apple-iTunes provider object hundreds of times, inflating the test bundle.
Creating a small helper, e.g.const APPLE_ITUNES = { name: 'Apple iTunes', logo: '/q6tl6Ib6X5FT80RMlcDbexIo4St.jpg' }
, and re-using it with the spread operator would make this file shorter, easier to edit, and less error-prone.Purely a maintainability suggestion – no functional impact.
tests/utils/search.ts (1)
27-39
: Guard againstundefined
when destructuringsearchQuery
If callers ever do
searchGet.searchMethod(undefined)
the destructuring
const { hybrid, ...rest } = searchQuery;
throws at runtime.
A tiny defensive tweak removes that foot-gun without altering typings:- searchMethod: (searchQuery) => { - const { hybrid, ...rest } = searchQuery; + searchMethod: (searchQuery = {}) => { + const { hybrid, ...rest } = searchQuery;src/indexes.ts (2)
126-134
: Skip calling the helper when no query params are provided
stringifyRecordKeyValues(undefined, ["filter"])
works only if the
helper defends againstundefined
. Making that explicit improves
readability and prevents future refactors from breaking:- params: stringifyRecordKeyValues(searchQuery, ["filter"]), + params: + searchQuery !== undefined + ? stringifyRecordKeyValues(searchQuery, ["filter"]) + : undefined,
298-299
: Type-only cast hides useful compiler checks
params as Omit<typeof params, "filter">
always collapses to{}
once
params
isundefined
, discarding the real shape ofDocumentsQuery
.
That defeats the purpose of typing the request parameters.Instead, keep the original type when
filter
is absent:- params: params as Omit<typeof params, "filter">, + // Safe: we are in the branch where `filter` is `undefined` + params: params as Exclude<DocumentsQuery<D>, { filter: unknown }>,This preserves the other query-string keys (
fields
,limit
, …) and
lets TypeScript flag accidental misspellings.src/types/types.ts (3)
112-116
: Name collision risk – two differentPagination
shapes exist now
src/types/search-parameters.ts
already exports aPagination
with{ page; hitsPerPage; }
.
Introducing anotherPagination
here ({ offset; limit; }
) with the same identifier but different shape can confuse consumers and tooling (auto-imports, code completion). Renaming this one to something likeOffsetPagination
or namespacing it would avoid ambiguity.
170-177
:retrieveVectors
flag lacks documentation & version guardThe new
retrieveVectors?: boolean
is great, but:
- Please add a JSDoc comment explaining that it retrieves stored vectors and requires Meilisearch ≥ 1.4 (or the correct minimum).
- Consider gating it behind a feature flag type‐side (e.g., conditional on
ExperimentalFeatures
) so users on older servers don’t inadvertently compile code that will 400.
370-375
: Possible type duplication –FieldDistribution
defined in multiple places
FieldDistribution
is now declared here, but it also exists insrc/types/search-response.ts
(imported by other modules). Having two identical declarations in different modules leads to structural-typing compatibility but breaksinstanceof
checks and confuses documentation generators. Prefer a single, shared definition inshared.ts
and re-export from here if needed.src/types/search-parameters.ts (1)
77-78
: Consider addressing the TODO commentThere's a TODO comment about potentially using generic records to type certain settings. This could be addressed in this PR or at least include more context about why it's being deferred.
Either implement the suggested approach or add more context to the TODO about the trade-offs and when this might be addressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/__snapshots__/search.test.ts.snap
is excluded by!**/*.snap
📒 Files selected for processing (29)
.code-samples.meilisearch.yaml
(15 hunks)README.md
(6 hunks)eslint.config.js
(1 hunks)playgrounds/javascript/src/meilisearch.ts
(2 hunks)src/http-requests.ts
(1 hunks)src/indexes.ts
(3 hunks)src/meilisearch.ts
(3 hunks)src/types/experimental-features.ts
(1 hunks)src/types/index.ts
(1 hunks)src/types/network.ts
(1 hunks)src/types/search-parameters.ts
(1 hunks)src/types/search-response.ts
(1 hunks)src/types/shared.ts
(2 hunks)src/types/token.ts
(1 hunks)src/types/types.ts
(7 hunks)src/utils.ts
(1 hunks)tests/client.test.ts
(1 hunks)tests/embedders.test.ts
(0 hunks)tests/env/node/getting_started.cjs
(1 hunks)tests/env/node/search_example.cjs
(1 hunks)tests/env/typescript-node/src/index.ts
(1 hunks)tests/facet_search.test.ts
(0 hunks)tests/get_search.test.ts
(0 hunks)tests/search.test.ts
(1 hunks)tests/token.test.ts
(2 hunks)tests/typed_search.test.ts
(0 hunks)tests/utils/meilisearch-test-utils.ts
(4 hunks)tests/utils/search.ts
(1 hunks)tests/utils/test-data/films.ts
(1 hunks)
💤 Files with no reviewable changes (4)
- tests/get_search.test.ts
- tests/typed_search.test.ts
- tests/embedders.test.ts
- tests/facet_search.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/types/token.ts (1)
src/types/search-parameters.ts (1)
SearchQuery
(147-149)
playgrounds/javascript/src/meilisearch.ts (1)
src/types/search-parameters.ts (1)
SearchQueryWithOffsetLimit
(126-126)
tests/client.test.ts (1)
tests/utils/meilisearch-test-utils.ts (3)
getClient
(275-275)clearAllIndexes
(265-265)config
(266-266)
tests/utils/meilisearch-test-utils.ts (1)
src/http-requests.ts (1)
T
(192-257)
src/types/search-parameters.ts (3)
src/types/shared.ts (4)
PascalToCamelCase
(21-21)SafeOmit
(29-29)RequiredKeys
(8-10)NonNullKeys
(4-6)src/types/types.ts (1)
Locale
(327-327)src/http-requests.ts (1)
T
(192-257)
src/meilisearch.ts (4)
src/types/search-parameters.ts (3)
MultiSearch
(199-199)FederatedSearch
(189-192)MultiSearchOrFederatedSearch
(202-202)src/types/types.ts (1)
ExtraRequestInit
(24-24)src/types/search-response.ts (3)
SearchResults
(135-137)FederatedSearchResult
(110-114)SearchResultsOrFederatedSearchResult
(140-142)src/types/experimental-features.ts (1)
RuntimeTogglableFeatures
(6-14)
src/types/search-response.ts (4)
src/types/shared.ts (2)
RecordAny
(2-2)DeepStringRecord
(23-27)src/http-requests.ts (1)
T
(192-257)src/types/search-parameters.ts (1)
Pagination
(45-50)src/types/types.ts (2)
FieldDistribution
(374-374)MeiliSearchErrorResponse
(460-467)
src/types/types.ts (1)
src/types/search-parameters.ts (2)
Pagination
(45-50)FilterExpression
(59-59)
tests/search.test.ts (7)
tests/utils/search.ts (5)
client
(13-13)INDEX_UID
(12-12)index
(14-14)search
(16-126)assertFacetDistributionAndStatsAreCorrect
(128-146)tests/utils/meilisearch-test-utils.ts (4)
HOST
(271-271)MASTER_KEY
(272-272)assert
(143-146)objectKeys
(264-264)src/meilisearch.ts (2)
index
(89-91)multiSearch
(229-238)tests/utils/test-data/films.ts (2)
FILMS
(16-887)Film
(1-14)src/types/search-parameters.ts (5)
SearchQuery
(147-149)MatchingStrategy
(20-20)FilterExpression
(59-59)SearchQueryWithIndexAndFederation
(158-161)Federation
(177-182)src/indexes.ts (4)
search
(105-114)searchGet
(125-134)searchSimilarDocuments
(149-158)searchSimilarDocumentsGet
(161-170)src/types/search-response.ts (4)
SearchResultWithOffsetLimit
(88-89)SearchResultWithPagination
(86-87)SearchHit
(29-38)ExplicitVectors
(23-26)
🪛 GitHub Check: codecov/patch
src/utils.ts
[warning] 24-25: src/utils.ts#L24-L25
Added lines #L24 - L25 were not covered by tests
src/meilisearch.ts
[warning] 428-431: src/meilisearch.ts#L428-L431
Added lines #L428 - L431 were not covered by tests
🔇 Additional comments (65)
tests/env/node/search_example.cjs (1)
27-29
: Updated search method call signature correctly implementedThe search method call has been updated to use the new unified signature that accepts a single options object with the query string under the
q
key, aligning with the API changes mentioned in the PR objectives.eslint.config.js (1)
44-49
: Good addition of test assertion verificationAdding the
vitest/expect-expect
rule ensures all test files include at least one assertion call, which prevents false-positive tests that don't actually verify anything. This is a good practice for maintaining test quality.src/http-requests.ts (1)
25-25
:✅ Verification successful
Simplified URL parameter handling
The parameter handling has been simplified by removing special
Date
object processing and using a more straightforward string conversion approach. This reduces code complexity while maintaining functionality.
🏁 Script executed:
#!/bin/bash # Check if Date objects were handled elsewhere in the codebase # Look for Date handling in parameter processing or serialization rg -A 2 -B 2 "instanceof Date" --type tsLength of output: 42
🏁 Script executed:
#!/bin/bash # Locate the http-requests.ts file HTTP_FILE=$(fd http-requests.ts) echo "Inspecting: $HTTP_FILE" # Display the first 100 lines to review parameter handling context sed -n '1,100p' "$HTTP_FILE" # Search within the file for any Date-specific logic rg -n "Date" "$HTTP_FILE"Length of output: 3276
🏁 Script executed:
#!/bin/bash # Locate the type definition file for URLSearchParamsRecord TYPE_FILE=$(fd --full-path "src/types/index.js") echo "Inspecting: $TYPE_FILE" # Show the definition of URLSearchParamsRecord rg -n "URLSearchParamsRecord" -C 5 "$TYPE_FILE"Length of output: 163
🏁 Script executed:
#!/bin/bash # Find the TypeScript file defining URLSearchParamsRecord FILE=$(rg -l "URLSearchParamsRecord" --type ts) echo "Definition file: $FILE" # Show the URLSearchParamsRecord type declaration rg -n "URLSearchParamsRecord" -C 5 "$FILE"Length of output: 369
🏁 Script executed:
#!/bin/bash # Properly iterate over files containing the type and show its declaration FILES=$(rg -l "URLSearchParamsRecord" --type ts) for file in $FILES; do echo "---- $file ----" rg -n "URLSearchParamsRecord" -C 5 "$file" doneLength of output: 1822
No Date-specific logic found; Date handling removal is safe
I searched the entire codebase and did not find any
instanceof Date
checks or other Date-specific serialization elsewhere, and theURLSearchParamsRecord
type only permits primitives (string, number, boolean, etc.), so dropping the special Date branch has no unintended impact.– No other Date handling detected in
src/http-requests.ts
or related files
– The simplifiedString(val)
conversion covers all permitted param typestests/token.test.ts (2)
335-336
: Updated search method signature for token validation testThe search method call has been updated to use the new unified signature with a
q
parameter, maintaining the test's ability to verify that searching with invalid token permissions correctly triggers an API key error.
353-354
: Updated search method signature for token validation testThe search method call has been updated to use the new unified signature with a
q
parameter, maintaining the test's ability to verify that searching with incorrect index permissions correctly triggers an API key error.src/types/token.ts (3)
1-1
: Well-structured type import updateThe import has been correctly changed from
Filter
toSearchQuery
to align with the renamed types mentioned in the PR objectives. This ensures consistency with the new type system.
4-4
: Good use of type utility for better type safetyUsing
Pick<SearchQuery, "filter">
to extract only the filter property from the largerSearchQuery
type is a good practice. It ensures type safety while keeping the interface minimal.
11-11
: Improved documentation referenceSimplifying the reference to the core implementation with a direct mention of
meilisearch_auth::SearchRules
instead of a GitHub URL makes the documentation more maintainable and less prone to becoming outdated.tests/env/node/getting_started.cjs (3)
30-30
: Updated search method signature as per PR changesThe search method call has been correctly updated to use the new signature with a single parameter object containing the query string under the
q
property.
32-36
: Proper implementation of filtered search with new signatureThe filtered search now correctly uses the new unified parameter object pattern, properly including both the query string and filter parameters in a single object.
38-42
: Faceted search correctly updated to new API formatThis faceted search correctly implements the new parameter structure, using an object with
q
,filter
(as an array), andfacets
properties. This is consistent with the API signature changes described in the PR objectives.tests/client.test.ts (2)
41-45
: Added test setup for experimental network featureGood addition of a
beforeAll
hook to enable the experimental network feature. This supports testing the new network-related functionality mentioned in the PR objectives.
47-51
: Proper cleanup of experimental settings after testsThe
afterAll
hook correctly disables the experimental feature before cleaning up indexes, ensuring tests don't leave side effects that could impact other test suites.src/types/index.ts (1)
1-6
: Good organization of expanded type exportsThese new exports provide a well-structured approach to exposing the refactored and new TypeScript types mentioned in the PR objectives. This ensures that all the new search-related types, experimental features, and network types are properly accessible throughout the codebase.
src/utils.ts (2)
19-35
: Nice utility function implementation!This generic utility function for stringifying specific keys in a record is well-typed and elegantly implemented. The type parameters ensure type safety while the mapped return type accurately preserves the original types for untouched keys and correctly annotates stringified values as strings.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-25: src/utils.ts#L24-L25
Added lines #L24 - L25 were not covered by tests
37-42
: Export organization is clean and clearThe exports are neatly organized, making it clear what utilities are available from this module.
src/types/network.ts (2)
1-9
: Well-documented Remote typeThe type definition includes helpful documentation links to the official API reference and corresponding Rust implementation. The structure with an optional API key accommodates different authentication scenarios.
11-19
: Network type looks goodThe Network type is well-structured to model both self-reference and remote connections. The documentation is clear and links to the appropriate references.
playgrounds/javascript/src/meilisearch.ts (3)
1-4
: Import updated correctly to include the new typeThe import has been properly updated to include the
SearchQueryWithOffsetLimit
type, which aligns with the PR objective to refactor the search method signatures.
42-47
: Search method signature updated appropriatelyThe implementation now correctly uses the new search method signature with a single parameter object containing
q
and other properties, as described in the PR objectives.
49-51
: Console output format updated to reflect parameter changesThe string template has been appropriately updated to display the new search parameter object structure.
.code-samples.meilisearch.yaml (7)
17-20
: Search parameter format updated correctlyThe date guide filter example has been updated to use the new search method signature with a single object parameter containing
q
andfilter
properties.
24-27
: Sort example properly convertedThe date guide sort example now correctly uses the object format with
q
andsort
properties.
83-85
: Basic search examples correctly updatedBoth the POST and GET search examples have been properly updated to use the new object format with the
q
property.
307-310
: Filter examples consistently updatedAll filter examples now correctly use the object parameter format with
q
andfilter
properties, maintaining consistency across the codebase.Also applies to: 312-315, 317-320
462-462
: Getting started example correctly updatedThe getting started search example correctly demonstrates the new API format.
771-774
: Advanced parameters example updated properlyThe example for
showRankingScoreDetails
has been correctly updated to the new parameter format.
789-795
: Hybrid search example is consistent with new formatThe hybrid search example follows the new parameter structure correctly, maintaining consistency with other examples.
src/types/experimental-features.ts (1)
1-14
: Well-structured type definition for experimental features.The
RuntimeTogglableFeatures
type is well-documented with references to API docs and backend structure. The optional boolean/null properties provide flexibility for feature toggling.tests/env/typescript-node/src/index.ts (4)
27-27
: Proper use of generic type parameter.Good use of generics to type the index with the
Movie
interface, providing type safety for search operations and hits processing.
34-36
: Search signature change correctly implemented.The code correctly uses the new search API that accepts a single parameter object with
q
property for the query string, aligning with the PR objectives to unify search method signatures.
41-43
: Correctly using updated search method.The search implementation follows the new pattern documented in the PR objectives, using a single parameter object instead of separate query and options parameters.
45-53
: Proper type handling for hits.The code correctly handles optional properties in search hits using the nullable chain operator, improving type safety. This is a good practice when working with search results.
tests/utils/meilisearch-test-utils.ts (5)
3-3
: Good addition of Task type import.Importing the
Task
type provides better type safety for the newisTaskSuccessful
helper function.
96-96
: Added complementary RESOLVED symbol.Adding the
RESOLVED
symbol complements the existingNOT_RESOLVED
symbol, creating a consistent pattern for assertion messages.
131-137
: Useful assertion helper for promises.The
resolves
assertion helper provides a clean way to assert that promises resolve without rejection, complementing the existingrejects
helper for comprehensive promise testing.
138-141
: Task validation helper improves test readability.The
isTaskSuccessful
assertion encapsulates common task success validation logic, making tests more concise and readable while enforcing consistent validation.
259-261
: Type-safe object keys extraction utility.The
objectKeys
function provides a type-safe way to extract keys from objects with string keys, enhancing type safety in testing code.src/types/shared.ts (5)
1-2
: Direct type definition improves maintainability.Defining
RecordAny
directly instead of importing it simplifies dependency management and makes the type more accessible.
4-10
: Useful utility types for refining object properties.The
NonNullKeys
andRequiredKeys
utility types provide powerful tools for transforming object types in a type-safe way, especially useful for API parameter validation.
20-21
: String case conversion utility with attribution.The
PascalToCamelCase
utility type with attribution to its source is a good practice. This utility will be helpful for converting between naming conventions in APIs.
23-27
: Recursive type mapping for deep objects.The
DeepStringRecord
type provides a robust way to recursively convert all properties in nested objects to strings, which is useful for serialization and display purposes.
29-29
: Type alias enhances code maintainability.The
SafeOmit
type alias provides semantic clarity and future-proofing. While currently a direct alias forOmit
, it establishes a pattern that could be extended with additional safety checks if needed.src/meilisearch.ts (1)
426-441
: Add unit-tests for the new experimental-features endpointsCode-coverage shows 0 % for the freshly added
getExperimentalFeatures
&updateExperimentalFeatures
paths.
A simple happy-path test (mocking the underlyingHttpRequests
) would keep these endpoints from going stale or accidentally breaking.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 428-431: src/meilisearch.ts#L428-L431
Added lines #L428 - L431 were not covered by testssrc/types/types.ts (2)
15-18
: Potential regression –URLSearchParamsRecord
can no longer acceptDate
instances
Date
was removed from the union. A lot of user-land code (including our own integration tests for tasks/jobs) relies on passingDate
objects directly to be subsequently stringified viaURLSearchParams
.
Please double-check usages across the repo – if any still expect to pass aDate
, they will now fail to compile.- string | string[] | number | number[] | boolean | null | undefined + | string + | string[] + | number + | number[] + | boolean + | Date // keep backward-compat + | null + | undefined
183-185
: Breaking change –DocumentsDeletionQuery.filter
is now mandatory
filter
moved from optional to required. This is logically correct (the endpoint refuses an empty body), but it is a breaking change for TypeScript users that previously called:index.deleteDocuments({});If we intend to keep the stricter typing, flag it prominently in the release notes; otherwise make it optional and let runtime validation handle the error.
README.md (5)
202-202
: LGTM: Updated search example with new API signatureThe search example now correctly uses the object parameter syntax with the
q
property instead of passing the query string directly, aligning with the PR's API changes.
230-233
: LGTM: Search parameters example updated correctlyThe example now properly demonstrates how to use the new search API with a single parameter object containing both the query and additional search parameters.
276-279
: LGTM: Filter example updated with new API signatureThe filter example correctly uses the new search API pattern with a single parameter object.
304-308
: LGTM: Placeholder search example updated properlyThe placeholder search example now correctly uses the new object parameter syntax while maintaining all the necessary properties.
350-350
: LGTM: Abortable search example updated correctlyThe abortable search example now uses the object parameter style for the query while keeping the request config as the second parameter.
src/types/search-parameters.ts (14)
1-8
: LGTM: Proper imports for utility typesThe file correctly imports utility types from shared.js and types.js that are used to build the comprehensive type system for search parameters.
9-14
: LGTM: Well-documented HybridQuery typeThe HybridQuery type is properly defined with the necessary properties and includes a reference to the corresponding Rust type in the Meilisearch backend.
15-21
: LGTM: MatchingStrategy type with proper transformationThe MatchingStrategy type correctly uses PascalToCamelCase to transform the Pascal case enum values from the backend to camel case for JavaScript, ensuring consistent typing while matching the expected API format.
22-37
: LGTM: FederationOptions type with experimental flagThe FederationOptions type is well-documented with links to the API documentation and backend type. The experimental remote property is properly marked with a comment, and the undocumented queryPosition property includes a private remark.
38-51
: LGTM: Clear separation of pagination approachesThe code correctly separates the two pagination approaches (offset/limit and page-based) into distinct types, each with proper documentation links to the API reference.
52-59
: LGTM: Flexible FilterExpression typeThe FilterExpression type properly handles the various formats that filters can be expressed in, with appropriate documentation links and private remarks about how it's handled in the backend.
61-117
: LGTM: Well-structured search query segmentsThe search query components are logically divided into segments that can be composed together, with comprehensive documentation links for each property. This approach makes the types more maintainable and easier to understand.
120-125
: LGTM: Elegant type transformation for GET requestsThe IntoGet type transformation elegantly handles the differences between POST and GET request formats, particularly for nested hybrid search parameters that need to be flattened for URL parameters.
126-154
: LGTM: Comprehensive search query type combinationsThese types provide a complete set of options for different pagination approaches and request methods, enabling strongly-typed API interactions while preserving flexibility.
139-145
: LGTM: Well-documented required pagination constraintThe enforcement of required pagination is well-documented with a clear explanation of why this constraint exists, relating it to the backend implementation and response shape guarantee.
156-171
: LGTM: Search query types with index integrationThese types correctly extend the base search query types to include index UID, enabling multi-search functionality while preserving type safety.
173-202
: LGTM: Well-structured federation typesThe federation-related types are well-structured and properly documented, providing a comprehensive type system for multi-search and federated search capabilities.
204-214
: LGTM: FacetSearchQuery with proper compositionThe FacetSearchQuery type correctly composes with the shared facet and search query segment, ensuring consistency across different query types.
215-226
: LGTM: SimilarQuery with enforced offset and limitThe SimilarQuery type properly composes multiple segments and uses NonNullKeys to enforce the presence of offset and limit properties, ensuring the API works correctly with this query type.
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Add test coverage for the undefined case
The static analysis tool indicates that the undefined handling branch is not covered by tests.
Consider adding a test case that specifically verifies the behavior when undefined is passed:
test('stringifyRecordKeyValues handles undefined input', () => {
const result = stringifyRecordKeyValues(undefined, ['filter']);
expect(result).toBeUndefined();
});
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for this utility function
rg "stringifyRecordKeyValues" --type=ts -A 5 -B 5
Length of output: 2078
Add missing test for the undefined branch in stringifyRecordKeyValues
The if (v === undefined) return;
path in src/utils.ts
isn’t covered by any existing tests. Please add a test to verify that passing undefined
yields undefined
.
• Location: src/utils.ts
(undefined‐handling branch around lines 24–25)
• Suggested test (e.g. in src/utils.test.ts
or your repo’s test suite):
import { stringifyRecordKeyValues } from './utils';
test('stringifyRecordKeyValues handles undefined input', () => {
expect(stringifyRecordKeyValues(undefined, ['filter'])).toBeUndefined();
});
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-25: src/utils.ts#L24-L25
Added lines #L24 - L25 were not covered by tests
): Promise<SearchResults>; | ||
multiSearch( | ||
federatedSearch: FederatedSearch, | ||
init?: ExtraRequestInit, | ||
): Promise<FederatedSearchResult>; | ||
async multiSearch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Parameter should be partial for update operations
updateNetwork()
right above correctly accepts a Partial<Network>
.
updateExperimentalFeatures()
should mirror this behaviour – the server accepts a sparse payload with only the toggles you want to touch. Requiring the full RuntimeTogglableFeatures
object forces callers to redundantly send the whole structure.
- async updateExperimentalFeatures(
- runtimeTogglableFeatures: RuntimeTogglableFeatures,
+ async updateExperimentalFeatures(
+ runtimeTogglableFeatures: Partial<RuntimeTogglableFeatures>,
): Promise<RuntimeTogglableFeatures> {
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
): Promise<SearchResults>; | |
multiSearch( | |
federatedSearch: FederatedSearch, | |
init?: ExtraRequestInit, | |
): Promise<FederatedSearchResult>; | |
async multiSearch( | |
async updateExperimentalFeatures( | |
runtimeTogglableFeatures: Partial<RuntimeTogglableFeatures>, | |
): Promise<RuntimeTogglableFeatures> { |
name: index.searchSimilarDocuments.name, | ||
searchMethod: async ({ | ||
offset, | ||
limit, | ||
...searchQuery | ||
}: SearchQueryWithOffsetLimit) => { | ||
const { hits, ...result } = await index.searchSimilarDocuments({ | ||
id: 607, | ||
embedder: "default", | ||
offset: offset ?? undefined, | ||
limit: limit ?? undefined, | ||
...searchQuery, | ||
}); | ||
|
||
return { | ||
...result, | ||
query: searchQuery.q ?? "", | ||
hits: hits as SearchHit<Film>[], | ||
}; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Avoid sending unsupported properties to searchSimilarDocuments
SimilarQuery
intentionally has a much smaller surface than a full SearchQuery
; spreading searchQuery
here risks forwarding keys such as hybrid
, vector
, attributesToSearchOn
, etc., that the endpoint neither documents nor accepts. When such keys are present the request will still compile (excess property checks are bypassed on spreads) but the HTTP call will silently drop them, making the test utility lie about what is really under test.
Consider whitelisting only the properties that are part of SimilarQuery
:
- const { hits, ...result } = await index.searchSimilarDocuments({
- id: 607,
- embedder: "default",
- offset: offset ?? undefined,
- limit: limit ?? undefined,
- ...searchQuery,
- });
+ const { hits, ...result } = await index.searchSimilarDocuments({
+ id: 607,
+ embedder: "default",
+ offset: offset ?? undefined,
+ limit: limit ?? undefined,
+ filter: searchQuery.filter ?? undefined,
+ rankingScoreThreshold: searchQuery.rankingScoreThreshold ?? undefined,
+ // add further whitelisted props when the API starts supporting them
+ } satisfies SimilarQuery);
This keeps the helper faithful to the public API surface and prevents accidental usage drift when new fields appear in SearchQuery
.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
name: index.searchSimilarDocuments.name, | |
searchMethod: async ({ | |
offset, | |
limit, | |
...searchQuery | |
}: SearchQueryWithOffsetLimit) => { | |
const { hits, ...result } = await index.searchSimilarDocuments({ | |
id: 607, | |
embedder: "default", | |
offset: offset ?? undefined, | |
limit: limit ?? undefined, | |
...searchQuery, | |
}); | |
return { | |
...result, | |
query: searchQuery.q ?? "", | |
hits: hits as SearchHit<Film>[], | |
}; | |
}, | |
name: index.searchSimilarDocuments.name, | |
searchMethod: async ({ | |
offset, | |
limit, | |
...searchQuery | |
}: SearchQueryWithOffsetLimit) => { | |
const { hits, ...result } = await index.searchSimilarDocuments({ | |
id: 607, | |
embedder: "default", | |
offset: offset ?? undefined, | |
limit: limit ?? undefined, | |
filter: searchQuery.filter ?? undefined, | |
rankingScoreThreshold: searchQuery.rankingScoreThreshold ?? undefined, | |
// add further whitelisted props when the API starts supporting them | |
} satisfies SimilarQuery); | |
return { | |
...result, | |
query: searchQuery.q ?? "", | |
hits: hits as SearchHit<Film>[], | |
}; | |
}, |
export type SimilarResult = { | ||
hits: SearchHit[]; | ||
id: string; | ||
} & ProcessingTime & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SimilarResult.id
should allow number
as well
SimilarQuery.id
is declared as string | number
, but SimilarResult
narrows the type down to string
. When callers pass a numeric id the
response will still contain that number, forcing them to cast or as unknown as
. Widen the type so the request/response cycle stays
consistent:
- id: string;
+ id: string | number;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export type SimilarResult = { | |
hits: SearchHit[]; | |
id: string; | |
} & ProcessingTime & | |
export type SimilarResult = { | |
hits: SearchHit[]; | |
id: string | number; | |
} & ProcessingTime & |
@@ -431,13 +425,13 @@ We welcome all contributions, big and small! If you want to know more about this | |||
#### [Make a search request](https://www.meilisearch.com/docs/reference/api/search) | |||
|
|||
```ts | |||
client.index<T>('xxx').search(query: string, options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>> | |||
client.index<T>('xxx').search(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type name in API reference needs to be updated
The type SearchParams
is referenced in the API documentation, but according to the PR description, this type has been renamed to SearchQuery
.
-client.index<T>('xxx').search(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
+client.index<T>('xxx').search(options: SearchQuery = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
client.index<T>('xxx').search(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>> | |
client.index<T>('xxx').search(options: SearchQuery = {}, config?: Partial<Request>): Promise<SearchResponse<T>> |
``` | ||
|
||
#### [Make a search request using the GET method (slower than the search method)](https://www.meilisearch.com/docs/reference/api/search#search-in-an-index-with-get-route) | ||
|
||
```ts | ||
client.index<T>('xxx').searchGet(query: string, options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>> | ||
client.index<T>('xxx').searchGet(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type name in API reference needs to be updated
The type SearchParams
is referenced in the API documentation for searchGet
, but according to the PR description, this type has been renamed to SearchQuery
.
-client.index<T>('xxx').searchGet(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
+client.index<T>('xxx').searchGet(options: SearchQueryGet = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
client.index<T>('xxx').searchGet(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>> | |
client.index<T>('xxx').searchGet(options: SearchQueryGet = {}, config?: Partial<Request>): Promise<SearchResponse<T>> |
Pull Request
Related issue
Fixes #1767
What does this PR do?
searchSimilarDocumentsGet
method (https://www.meilisearch.com/docs/reference/api/similar#get-similar-documents-with-get)multiSearch
, because it was limited to generalizing document shape to only one index, whilemultiSearch
can search or even combine documents from multiple indexes, and it is not practical to try and generalize thosehits as SearchHit<YourType>[]
orhits as FederatedSearchHit<YourType>[]
)Migration
client.index(...).search
now has only two parameters, it requiresq
parameter to be inside the old second parameter, which is now the first parameter.multiSearch
no longer accepts TypeScript generics.Types:
MatchingStrategies
->MatchingStrategy
Filter
->FilterExpression
Query
Highlight
Crop
SearchForFacetValuesParams
->FacetSearchQuery
FacetHit
->FacetValueHit
SearchForFacetValuesResponse
->FacetSearchResult
HybridSearch
->HybridQuery
SearchParams
->SearchQuery
SearchRequestGET
->SearchQueryGet
MultiSearchQuery
->SearchQueryWithIndex
MultiSearchQueryWithFederation
->SearchQueryWithIndexAndFederation
MultiSearchParams
->MultiSearch
FederatedMultiSearchParams
->FederatedSearch
CategoriesDistribution
->FieldDistribution
Facet
FacetDistribution
RankingScoreDetails
->ScoreDetails
Hit
->SearchHit
Hits
FacetStat
->FacetStats
FacetStats
FacetsByIndex
->FederatedFacets
SearchResponse
->SearchResult
MultiSearchResult
MultiSearchResponse
->SearchResults
MultiSearchResponseOrSearchResponse
->SearchResultsOrFederatedSearchResult
SearchSimilarDocumentsParams
->SimilarQuery
Summary by CodeRabbit
New Features
Improvements
q
property.Bug Fixes
Tests
Chores